Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: Fix keyboard navigation with scroll_to_article_header=0 #1473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PhrozenByte
Copy link
Contributor

Keyboard navigation is virtually impossible with scroll_to_article_header=0, because buttons like Space also move the view point. We could also prevent the browser from moving the view point, but I believe that visually scrolling to the selected entry always is the expected behaviour when using keyboard navigation.

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for selfoss canceled.

Name Link
🔨 Latest commit 0e59b31
🔍 Latest deploy log https://app.netlify.com/sites/selfoss/deploys/656354b30c9430000808274b

@jtojnar
Copy link
Member

jtojnar commented Nov 26, 2023

Keyboard navigation is virtually impossible with scroll_to_article_header=0, because buttons like Space also move the view point.

I cannot reproduce this in either Firefox or Chromium – we are already preventing the default browser behaviour:

event.preventDefault();

The only time the space should move the viewport is when the next item does not fit into the viewport:

autoScroll(currentElement);

if (viewportScrollTop + viewportHeight < targetTop + targetHeight + 80) {

but I believe that visually scrolling to the selected entry always is the expected behaviour when using keyboard navigation.

Hmm, I guess that depends on how you use selfoss. Personally, I read multiple item titles in a row and only open those that I find interesting – when it opens in place, I can just continue reading on the next line. Of course there will still be jump when near the end but that is inevitable.

Also, as is the PR makes it scroll to article header even when clicking a title, not just for keyboard navigation.

@PhrozenByte
Copy link
Contributor Author

PhrozenByte commented Nov 26, 2023

Something weird is going on here 🙈 I definitely once had the issue that Space was moving the viewport, but I no longer can reproduce that. Maybe it was just some intermediate issue while developing my other PRs.

However, I did write this fix after all my other PRs, thus there definitely still is a similar issue with moving the viewport - and after some research I managed to reproduce it. Due to your explanations (thanks! 👍) I now also understand what the issue is: It's not that Space moves the viewport, but there's an issue with autoScroll(), so I might just have mixed these up.

Assume you have four entries: The 1st, 2nd and 4th all consist of just a single line of content. The 3rd in contrast is huge, longer than the screen. You hit Space and the first entry opens, easily fitting into the screen. You hit Space again, collapsing the first and opening the second entry; both still easily fitting into the screen. You hit Space again, the second entry collapses and the third entry opens, now overflowing the screen. Hitting Space again sends your viewport way down, because it calculates the target viewport based on this huge 3rd entry that wasn't collapsed yet, but which is about to collapse.

In native JavaScript we just have to wait for another animation frame, i.e. wrap this

const currentElement = document.querySelector(`.entry[data-entry-id="${current}"]`);
// scroll to element
autoScroll(currentElement);
// focus the title link for better keyboard navigation
currentElement.querySelector('.entry-title-link').focus();

into window.requestAnimationFrame(() => { /* */ });. I just commited that (see force-push of 0e59b31) - and it works now.

However, I still know next to nothing about React, so I might rather use some feature provided by React instead, I just don't know which. Just let me know, I'll change it accordingly. 👍

Also, as is the PR makes it scroll to article header even when clicking a title, not just for keyboard navigation.

This happens when one writes patches in a hurry and doesn't properly test it 😒 Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants